I am not sure I like the design here. The IOVA address space, being virtual, can have aliases. Could you still store the bitmap in the low-level memory region, but with accesses going through IOMMU translation?

What does vhost-user require? Are dirty bitmap accesses done in IOVA or GPA space?

Are dirty bitmap accesses done in IOVA or GPA space?

They’re done in IOVA space, which is why I think we need the bitmap on this level.

(If we continued to store it in e.g. GuestRegionMmap/MmapRegion, it would need a reverse translation, and then keep a mapping of address ranges to bitmap slices instead of just a single bitmap slice linearly covering the entire region.)

They’re done in IOVA space, which is why I think we need the bitmap on this level.

I see. Maybe you could add a bitmap::BS<'a, Self::Bitmap> as another argument that try_access() passes back to the callback? And then IommuMemory::get_slice() can pass that argument to replace_bitmap().

This way, the Iommu controls whether dirty tracking is done in IOVA or GPA space.

You mean putting the responsibility on the try_access() callback to dirty a bitmap slice given to it if it has written anything?

I’m not sure, that does sound more than reasonable, but would require more changes in the callers than just to add the Permissions flag. 🙂

You mean putting the responsibility on the try_access() callback to dirty a bitmap slice given to it if it has written anything?

No, I confused try_access and get_slice sorry. For IoMemory, dirtying is already done it in try_access itself; for Iommu the slice could be returned by translate, that is it would be part of the Iotlb entry?

I’m sorry, I don’t quite follow; currently, dirtying is done only by IommuMemory (in its IoMemory implementation), neither IoMemory nor Iommu.

<IommuMemory as IoMemory>::try_access() dirties the bitmap itself, right.

<IommuMemory as IoMemory>::get_slice() has the VolatileSlice do the work. For this, it replaces the VolatileSlice’s internal bitmap slice (which is initially set by the GuestMemoryRegion) by the right slice in the IOVA space.

I don’t think the IOMMU object should do any of this, and I don’t think it should return slices (if I understand correctly). I think it should only do the translation and not care about the actual memory.

::get_slice() has the VolatileSlice do the work. For this, it replaces the VolatileSlice’s internal bitmap slice (which is initially set by the GuestMemoryRegion) by the right slice in the IOVA space.

To clarify, I was saying it should not be the IommuMemory that decides the address space used by the bitmap - whether IOVA space as vhost wants or GPA space as everyone else (?) wants. I thought Iommu would return the slice in the Iotlb object, but I agree that probably Iommu isn't the right place either.

That said I don't care much, because as long as IommuMemory is hidden behind a feature, it's just a convenience or a sample implementation (and QEMU probably will not use IommuMemory, only IoMemory). Maybe just add a comment that says "NOTE: IommuMemory can only be used if the dirty bitmap tracks accesses in IOVA space".

Maybe just add a comment that says "NOTE: IommuMemory can only be used if the dirty bitmap tracks accesses in IOVA space".

To be precise, it depends on the IOMMU enabled setting: With it enabled, it’s IOVA space, otherwise it’s whatever the address space of the underlying GuestMemory is (for vhost, that would be VUA).

I could speculate on how it works for non-vhost (specifically in-hypervisor) cases; maybe the underlying memory will use GPA in that case, and therefore we can (and have to) track dirty accesses in GPA then. To make that work, we would only need a separate flag to control whether the IOVA dirty bitmap is supposed to be used or not.

But I think the best would be for me not to speculate and just leave this as it is – if such a separate flag would be enough, we can just add it later. For the time being, as you suggest, a comment that specifies the dirty bitmap behavior should be enough.

Why? Yes, the current implementations do provide that. But is it something we need the trait to require?

I think so, because you want any other implementations to be consistent. It's just a marker.

If another implementation can’t provide FusedIterator for free, this would incur overhead, though.

As far as I understand FusedIterator, users who need it can just use .get_slices()?.fused(), and this should be zero-cost if the concrete type does implement FusedIterator, no?

Yes but I think generally speaking fused behavior is desirable. Chances are that users will expect fused behavior and work only on some backends. (Also I don't see how a IoMemorySliceIterator could magically restart providing values after a None... almost all iterators should be fused IMO).

I don’t know what to say, that’s just wrong. (Edit: “that” being assuming all iterators are fused)

As I see it, Rust as a language has chosen iterators to generally not be fused, and requiring it in this trait would go against that design decision.

You're right, that was not really a great way to put it so let me rephrase.

IoMemorySliceIterator is returned by get_slices(). Users of get_slices() should know what semantics to expect if the iterator starts providing items again after None. The documentation should say that, while making it clear that returning an iterator with fused behavior is acceptable too.

If there is a reasonable description of what to expect, then the current code is fine but the documentation for get_slices() is lacking. I don't think this is the case, the documentation is fine.

But then, returning a FusedIterator should be part of get_slices()'s contract, and + FusedIterator should be in either get_slices() or IoMemorySliceIterator. The former seems unnecessarily pedantic, so I'd put it in the trait.

That is, for unfused iterators to be useful, you need to know when to try calling .next() again and what the result would be. Generally they are used for "resuming" iterators, like std::sync::mpsc::TryIter. I don't think get_slices() should return such a resuming iterator.

OK, I’ll add the FusedIterator bound, but I disagree with this specific reasoning. In short: I still don’t think it’s actually useful, but I think it’s necessary.

That is, for unfused iterators to be useful, you need to know when to try calling .next() again and what the result would be.

I really don’t see why that would be the case. You run .next() until it returns None, then you don’t use the iterator any more. In my experience, that is the absolute vast majority of use cases, and it doesn’t matter how the iterator behaves after the first None.

Consequently, I think it can be fine for the documentation to leave open what next() does once iteration is finished if that is deliberately left implementation-defined. However, of course, clearer documentation covering more edge cases is always better.

As for users requiring fused behavior, FusedIterator’s documentation (as I understand it) says not to rely on bounds, but always use .fuse().

I also disagree with the implication that if the iterator isn’t fused, it must be resuming. As you say, “generally” non-fused iterators could be resuming, not always. Other behavior is perfectly legal if it’s deemed more useful, and I would say largely undefined behavior is also OK if absolutely required for optimization purposes.

Having said all that, what you do make me see is that I should deliberate what behavior I actually do want. Implementation-defined (effectively undefined) behavior comes at a cost, I can’t just choose this by default because I’m lazy: Safe code is allowed to call .next() after the iterator is exhausted, and while that is wrong if you don’t know what to expect, it’s still legal safe code, so it is even more wrong for me to shrug my shoulders and say “whatever, don’t do that”.

Now, if such undefined behavior actually had a good reason to it, e.g. “It’s really important for performance here”, OK, but I don’t have such a reason. The only actual reason I have is that once we add that bound, removing it would be an incompatible change. Not very strong.

So yes, I really should at least consider what behavior makes sense after iterator exhaustion, and specify it. And here I agree that fused behavior is indeed the only behavior that does make sense, so let’s add that bound.

Sounds good, in fact I agree with all that you wrote. :) Including this:

I still don’t think it’s actually useful, but I think it’s necessary.

Thanks for writing it down better than I did.

Add a getter too?

Sure!

So if I understand correctly you'd do something like

    // `as` is a GuestMemoryAtomic
    let g = as.lock().unwrap();
    let new_iommu = as.load().inner_replaced(new_memory);
    g.replace(new_iommu);

Maybe we can add an

impl<M: GuestMemory, I: IOMMU>
    GuestMemoryExclusiveGuard<'_, IommuMemory<M, I>> {
    /// Replace the memory map in the `GuestMemoryAtomic` that created
    /// the guard with the new memory map, `map`, while keeping the IOMMU.
    /// This method consumes the guard.
    pub fn replace_backend(self, map: M) {
        let old = self.parent.inner.0.load();
        let new = old.inner_replaced(map);
        self.replace(new);
    }
}

(and TBH I'd love for inner_replaced to be pub(crate) but maybe that's asking too much).

Yes, that’s the pattern.

(In practice, inner_replaced() is called through a helper trait (VirtualMemory) that vhost-user-backend implements on both IommuMemory and GuestMemoryMmap, which provides a with_physical_memory() method that uses inner_replaced() for IommuMemory and just returns the new memory for GuestMemoryMmap.)

Why do you want to add that method?

Also, it isn’t generic across IoMemory, so it could only be used specifically for IommuMemory. I suppose we could add a trait Foo<M: IoMemory> with this method, and implement it for multiple GuestMemoryExclusiveGuards<M>. Then, vhost-user-backend could drop that with_physical_memory() method (but still needs the trait anyway). That alone doesn’t seem worth the effort here to me, but that’s why I’m wondering why you would like to have that method.

Why do you want to add that method?

Because right now GuestMemoryAtomic focuses on replacing the whole GuestMemory, but there is a usecase for replacing the backend only.

Also, it isn’t generic across IoMemory, so it could only be used specifically for IommuMemory

Yes, it's an extra impl only for IommuMemory.

That alone doesn’t seem worth the effort here to me, but that’s why I’m wondering why you would like to have that method

More for clarity/documentation than anything else. inner_replaced was a bit mysterious (maybe it's just the name, with_replaced_backend or anything along those lines would help me too).

I see now that it can't be pub(crate) though!

there is a usecase for replacing the backend only.

True, but if we wanted to really provide for that use case, we’d need a generic IoMemory implementation (at least for all types in vm-memory).

inner_replaced was a bit mysterious (maybe it's just the name, with_replaced_backend or anything along those lines would help me too).

Sure, I’d be more than happy to (and also rename .inner() to .get_backend()). I’ll definitely do this.

Ok, let's do the rename and agree to disagree on the rest. :)

I'd rather call this get_mmap() and do the clone itself returning an Arc<MmapRegion<B>>.

Sure!

I'll add a note here for simplicity, but it applies to the whole commit.

There are cases (e.g. QEMU :)) in which you want to track at the backend level independent of whether you have an IOMMU or not. The vhost case is the weird one due to its usage of VMM userspace addresses. I'd prefer that the comments and commit messages put less emphasis on this, explaining that there are simply two levels of bitmaps.

Related to this, I'm not super enthusiastic about forcing the usage of Arc<> for the bitmap, because having an Arc<()> in the simple case is a bit weird. However, all the derefs should be optimized away so I guess it's fine.

I'd prefer that the comments and commit messages put less emphasis on this, explaining that there are simply two levels of bitmaps.

I understood this type to be specific to vhost’s model, hence this note saying so. Now, it will probably be easy to modify it slightly to allow for QEMU’s model as well, basically IommuMemorySliceIterator::do_next() would just have to skip replacing the VolatileSlice’s bitmap. But I don’t see the point in doing that now, as I don’t have the knowledge if this is really what’s needed (and I can’t test it), and such a change could be a compatible one anyway[1].

So I don’t know what you would like to change specifically. Of course I can make it clear in the commit message that this is just for the vhost case, but I’m a bit lost beyond that.

[1] Add a method disable_virtual_bitmap(), pass this state on to all IommuMemorySliceIterators having them skip replacing the bitmap, and this note would be modified to say “If you don’t want that, call disable_virtual_bitmap()”. Yes, if “not wanting that” is the default case, that’s a bit weird, but not the end of the world. But doing anything else (e.g. implement that functionality now, adding a boolean to IommuMemory::new()) would require someone to vouch that that’s really what’s needed and that it would really be useful.

I understood this type to be specific to vhost’s model, hence this note saying so.

It is not used by QEMU but it may be used by other users than vhost. This handling of dirty bitmap is not even required for IommuMemory; I just want to make it clear that it's vhost that needs to have the bitmaps organized this way. IommuMemory allows that but it's possible to use IommuMemory while having bitmaps only at the physical address level.

But doing anything else (e.g. implement that functionality now, adding a boolean to IommuMemory::new()) would require someone to vouch that that’s really what’s needed and that it would really be useful.

Absolutely. All that I'm asking is to not "discount" the case where the IommuMemory's bitmap is () in the documentation.

Of course I can make it clear in the commit message that this is just for the vhost case

Is it clearer now?

I just want to make it clear that it's vhost that needs to have the bitmaps organized this way.

I.e. modify the comment to not just say “this type should only be used when this is the desired behavior” but explicitly something like “This is specifically the vhost-user model”?

All that I'm asking is to not "discount" the case where the IommuMemory's bitmap is () in the documentation.

Hard, because the bitmap between the back-end and IommuMemory must be the same type, or we couldn’t replace it in VolatileSlice. Would you like the bitmap to be wrapped in Option<>?

Hard, because the bitmap between the back-end and IommuMemory must be the same type, or we couldn’t replace it in VolatileSlice.

Ouch, I see. Yeah, change the comment so that others don't fall into the same mistake as me and make it clearer that IommuMemory wants the two-bitmap scenario.

Do you think it would be useful to have the bitmap be optional?

If you have any ideas we can have a separate PR before the 0.18.0 release. Overall for IommuMemory it's okay, as the whole struct is just a convenience.

OK, makes sense.